Create Networking Service #49
Conversation
ec2
requested review from
GregTheGreek,
austinabell,
ansermino and
dutterbutter
…e previous commit
| #[behaviour(out_event = "MyBehaviourEvent", poll_method = "poll")] | ||
| pub struct MyBehaviour<TSubstream: AsyncRead + AsyncWrite> { | ||
| pub gossipsub: Gossipsub<TSubstream>, | ||
| pub mdns: Mdns<TSubstream>, | ||
| #[behaviour(ignore)] |
There was a problem hiding this comment.
I am curious what #[behaviour(ignore)] and #[behaviour(out_event = "MyBehaviourEvent", poll_method = "poll")] do? Can you explain?
There was a problem hiding this comment.
#[derive(NetworkBehaviour)] imparts the NetworkBehaviour trait to MyBehaviour. Then #[behaviour(out_event = "MyBehaviourEvent", poll_method = "poll")] are the attributes that the macro expects, to give it more context.
| }, | ||
| } | ||
|
|
||
| impl<TSubstream: AsyncRead + AsyncWrite> NetworkBehaviourEventProcess<MdnsEvent> |
There was a problem hiding this comment.
I think it would be beneficial to have comments for some of these functions below
There was a problem hiding this comment.
These functions are all inherited through a trait. Do you think that it would still need documentation? Also they are not functions that we EVER should call. They are called by Libp2p
| config | ||
| .listening_multiaddr | ||
| .parse() | ||
| .expect("Incorrect MultiAddr Format"), |
There was a problem hiding this comment.
function returns a result, are you sure you want this to be a panic?
There was a problem hiding this comment.
Yeah. I want it to panic if the user gives an invalid address to listen on.
| .parse() | ||
| .expect("Incorrect MultiAddr Format"), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
same here, the function signature is for a result, but there are no error types returned anywhere in the function. If intended to be a panic for these, change the function signature to just return Self
There was a problem hiding this comment.
Yeah, you're right. Ill change that to return just Self
There was a problem hiding this comment.
Yeah. I want it to panic if the user gives an invalid address to listen on.
| )) | ||
| .map(|(peer, muxer), _| (peer, core::muxing::StreamMuxerBox::new(muxer))) | ||
| .timeout(Duration::from_secs(20)) | ||
| .map_err(|err| Error::new(ErrorKind::Other, err)) |
There was a problem hiding this comment.
why are you changing the error to an Other io error? Are you sure this is the type of error this should return?
There was a problem hiding this comment.
Yeah. We get some sort of libp2p transport error, and i want to convert it to an IO error
There was a problem hiding this comment.
I think it would be in our best interest to have these functions handle errors instead of panicing at random points unhandled. It may not be an issue now, but it would cause headaches when those panics hit when we are relying on certain services to complete or clean data on other threads.
One way to make the error handling pretty basic is to just have String error types and whenever an error is handled, map the error as such .map_error(|e| format!("<Some error text>: {}", e))?; since the errors don't need specific types or be checked as they are eventually being printed to the console.
One other thing that would be nice is handling sigints/ Ctrl+c, but less important than the error handling.
node/src/main.rs
Outdated
| // Create the tokio runtime | ||
| let rt = Runtime::new().unwrap(); | ||
|
|
||
| // Create the channel so we can receive messages from NetworkService | ||
| let (tx, _rx) = mpsc::unbounded_channel::<NetworkEvent>(); | ||
| // Create the default libp2p config | ||
| let netcfg = Libp2pConfig::default(); | ||
| // Start the NetworkService. Returns net_tx so you can pass messages in. | ||
| let (_network_service, _net_tx, _exit_tx) = | ||
| NetworkService::new(&netcfg, log.clone(), tx, &rt.executor()); | ||
|
|
||
| rt.shutdown_on_idle().wait().unwrap(); |
There was a problem hiding this comment.
Does it make sense to have this logic in a run function instead of everything specifically in the main function? imo better for future proofing and if you want to handle Errors better than panicing whenever any error is hit (which I don't think is a pattern we should keep in the future)
There was a problem hiding this comment.
Potentially. I feel like its not worth exploring taking this out into a function until we have more services up?
Also w.r.t. errors, the only time we panic is when we can't recover from an error. In node specifically, we unwrap 5 times and expect 2 times. 1 of the unwraps and 1 of the expects is if we cant parse the multiaddr we wanna listen on. Not sure how you would handle that except for panicking. 2 other unwraps is addressed before (has to do with mutex poisioning, which should panic). The other 2 unwraps are if the executor fails to start, which is definitely something we don't know how to handle and should probably never happen. If it happens, your computers messed up probably.
There was a problem hiding this comment.
Also worth noting is that all of these unwraps and expects (except for the mutex ones) do not happen in random points in time. They will error at node bootup time.
There was a problem hiding this comment.
Also, note on SIGINTs, i will make a PR for that soon 👌
| use tokio::sync::mpsc; | ||
|
|
||
| use ferret_libp2p::config::Libp2pConfig; | ||
| use ferret_libp2p::service::NetworkEvent; | ||
| use network::service::*; | ||
|
|
||
| use futures::prelude::*; | ||
|
|
||
| use tokio; | ||
|
|
||
| use tokio::runtime::Runtime; |
There was a problem hiding this comment.
one last nit and I'm good :D
mDNS and Gossipsub are in there. Can also pass in a Libp2pConfig struct to start the Network service with a set of bootnodes, pubsub topics, and ip/port for listening. Remaining is adding Kademlia. Going to make adding Kademlia a separate PR since this is getting quite big.
Closes